Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configured endpoint URL resolution #2973

Merged
merged 9 commits into from
Jul 5, 2023
Merged

Conversation

kdaily
Copy link
Member

@kdaily kdaily commented Jun 20, 2023

This PR implements the proposal for configuring endpoints per service:

aws/aws-sdk#230

Adds a config provider to resolve the endpoint URL provided in an environment variable or shared configuration file. It resolves the endpoint in the following manner:

  1. The value provided through the endpoint_url parameter provided to the client constructor.
  2. The value provided by a service-specific environment variable.
  3. The value provided by the global endpoint environment variable (AWS_ENDPOINT_URL).
  4. The value provided by a service-specific parameter from a services definition section in the shared configuration file.
  5. The value provided by the global parameter from a services definition section in the shared configuration file.
  6. The value resolved when no configured endpoint URL is provided.

The endpoint config provider uses the client name (name used to instantiate a client object) for construction and add to the config value store. This uses multiple client/service name lookups to handle service name changes for backwards compatibility.

@kdaily kdaily requested a review from nateprewitt June 20, 2023 16:08
@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2023

Codecov Report

Patch coverage: 97.97% and project coverage change: +0.04 🎉

Comparison is base (13468bc) 93.32% compared to head (2fa2224) 93.36%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2973      +/-   ##
===========================================
+ Coverage    93.32%   93.36%   +0.04%     
===========================================
  Files           65       65              
  Lines        13764    13840      +76     
===========================================
+ Hits         12845    12922      +77     
+ Misses         919      918       -1     
Impacted Files Coverage Δ
botocore/config.py 100.00% <ø> (ø)
botocore/configprovider.py 94.78% <96.82%> (+0.68%) ⬆️
botocore/args.py 100.00% <100.00%> (ø)
botocore/configloader.py 100.00% <100.00%> (+2.08%) ⬆️
botocore/handlers.py 95.21% <100.00%> (ø)
botocore/session.py 97.65% <100.00%> (+0.01%) ⬆️
botocore/utils.py 79.43% <100.00%> (+0.02%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kdaily kdaily requested a review from kyleknap June 20, 2023 17:32
botocore/configprovider.py Outdated Show resolved Hide resolved
botocore/configprovider.py Outdated Show resolved Hide resolved
botocore/configprovider.py Outdated Show resolved Hide resolved
botocore/configprovider.py Outdated Show resolved Hide resolved
botocore/configprovider.py Outdated Show resolved Hide resolved
@kdaily kdaily requested a review from nateprewitt June 20, 2023 23:41
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 🚢

Copy link
Contributor

@nateprewitt nateprewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@larroy
Copy link

larroy commented Jun 22, 2023

Will this work for setting the S3 endpoint url?

@A-Levin
Copy link

A-Levin commented Jun 23, 2023

when will you merge it?

@kdaily kdaily force-pushed the configured-endpoint-urls branch 2 times, most recently from 63cb45d to 9ba6fdd Compare June 28, 2023 16:47
When deep copying the config value store, override values were not
preserved.
Instead of deep copying, use a shallow copy of the config value store
when using it for creating a new client from a session. Only use a deep
copy for updating the section provider.

Update behavior of the smart defaults update functions to use a copy
of the existing config providers in all cases. Update the logic for
determining which new provider to create.

Move unit tests for config provider smart defaults to functional tests
and rework them to better test behavior. A mocked defaults data file is
used to confirm that the correct values are loaded to protect against
changes to the live data file.
Adds a config provider to resolve the endpoint URL provided in an
environment variable or shared configuration file. It resolves the
endpoint in the following manner:

1. The value provided through the `endpoint_url` parameter provided to
   the client constructor.
2. The value provided by a service-specific environment variable.
3. The value provided by the global endpoint environment variable
   (`AWS_ENDPOINT_URL`).
4. The value provided by a service-specific parameter from a services
   definition section in the shared configuration file.
5. The value provided by the global parameter from a services definition
   section in the shared configuration file.
6. The value resolved when no configured endpoint URL is provided.

The endpoint config provider uses the client name (name used to
instantiate a client object) for construction and add to the config
value store. This uses multiple lookups to handle service name changes
for backwards compatibility.
The tests use a data file to load tests that enumerate the ways that the
endpoint URL can be defined and asserts that the correct endpoint is
used in a request and that it is ignored correctly if the appropriate
shared config file property or environment variable is supplied.

They also assert that the correct config section name and environment
variable name are used to configure and override the endpoint URL for
every AWS service.
Add a debug statement when specifically ignoring the configured
endpoint URL from the shared configuration file or an environment
variable. This will not be logged if the endpoint URL is set through
the client constructor.
Simplify docstring and fix typos.
Add tests to check that setting the client creation parameter to ignore
the configured endpoint URLs is respected.
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good just had a small comment on the test update

)


def _known_service_names_and_models():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this to _known_service_names_and_ids() to be consistent with the return value

GitHub Actions was consistently failing on Windows instances with this
test. Likely cause was loading the large model files into memory, which
persisted until testing was completed. Since the rest of the model is
not needed, only return the service name and ID.
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants